-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PC-1344: Update C# and .NET #365
Conversation
} | ||
else | ||
{ | ||
using var reader = new StreamReader(PostcodeJsonPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
favour throwing an error in the case that this one doesn't exist
4da97c0
to
3d26601
Compare
<PackageReference Include="CabinetOffice.GovUkDesignSystem" Version="1.0.0-456b8e9" /> | ||
<PackageReference Include="CabinetOffice.GovUkDesignSystem" Version="1.0.0-3f67920" /> | ||
<PackageReference Include="CsvHelper" Version="30.0.1" /> | ||
<PackageReference Include="GovukNotify" Version="6.1.0" /> | ||
<PackageReference Include="Hangfire.AspNetCore" Version="1.8.0" /> | ||
<PackageReference Include="Hangfire.Core" Version="1.8.0" /> | ||
<PackageReference Include="Hangfire.PostgreSql" Version="1.19.12" /> | ||
<PackageReference Include="Hangfire.AspNetCore" Version="1.8.14" /> | ||
<PackageReference Include="Hangfire.Core" Version="1.8.14" /> | ||
<PackageReference Include="Hangfire.PostgreSql" Version="1.20.9" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to still upgrade(?) these dependencies as part of this ticket? I think this might be a remnant of the original troubleshooting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have reverted; will see if pipeline still works fine
<PackageReference Include="CabinetOffice.GovUkDesignSystem" Version="1.0.0-456b8e9" /> | ||
<PackageReference Include="CabinetOffice.GovUkDesignSystem" Version="1.0.0-3f67920" /> | ||
<PackageReference Include="Community.Microsoft.Extensions.Caching.PostgreSql" Version="3.1.2" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> | ||
<CopyToPublishDirectory>PreserveNewest</CopyToPublishDirectory> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need both of these? Have you tested without?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems not
// build structure means json file is in different location when running locally | ||
private const string PostcodeJsonPath = "Services/EligiblePostcode/EligiblePostcodeData.json"; | ||
|
||
private const string LocalPostcodeJsonPath = | ||
"../HerPublicWebsite.BusinessLogic/Services/EligiblePostcode/EligiblePostcodeData.json"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to come back to this if we still need to later: it feels brittle to have the paths hard-coded here and I think we'd want a better way to tell if we're running the app locally than checking if the file exists at that path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should now be resolved; resources are loaded in the same way between local and deployed
jsonContents = reader.ReadToEnd(); | ||
} | ||
|
||
eligiblePostcodes = JsonConvert.DeserializeObject<List<string>>(jsonContents); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this approach. We've got
services.AddScoped<IEligiblePostcodeService, EligiblePostcodeService>();
in Startup.cs
, which I believe means we'll get a fresh instance of this class per request. I'm concerned about the memory demands if we're reading the JSON and storing in memory for each instance. Previously this wouldn't have been an issue because the list was a static property, so we only ever had one copy in memory.
If we're keeping the postcodes in the codebase, we'd like to have a single copy of the data pulled into memory at build-time, or otherwise when first needed. But that's still potentially quite a load. Did you think about storing them in the database?
This is the sort of thing that it's probably worth getting wider opinions on around Softwire, for what it's worth. I'm happy to proof-read the question before you ask it if you like: I expect the way you ask it will have quite a big impact on how helpful the answers are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for what it's worth the full length of the postcode JSON file is 5.5mb, so I wasn't super convinced this would be too taxing on application memory too much
agree the approach could be improved. I did a bit of research into including the data at build time from a JSON though couldn't find much online about it, other than including the file and loading it an runtime. looks like AddSingleton
would make it so a single instance is created https://learn.microsoft.com/en-us/dotnet/api/Microsoft.Extensions.DependencyInjection.ServiceLifetime?view=net-8.0
could do a database col for this? though including the data in the database automatically sounds difficult, since we either need to load the json when running the migration (hard to test) or include it in the migration code (which could lead to the same issue as before)
can draft up a question if you'd like, just want to be clear on what our constrains and considerations are here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So yes, we can have a singleton service. I think we'd want to extract just the postcodes into that service, rather than the whole thing.
Database table we'd set up via migration: I wouldn't expect it to cause the same issue, but I suppose it's possible we might have to be a bit creative with the migration - not a guarantee.
It's entirely possible the memory impact isn't really something to worry about - I don't have a great heuristic for this. That's one of the things you could ask about. Will want to think about scalability though + the difficulty is that we don't have a good way to test out if it would be an issue before releasing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a part of our ASP.NET service that checks if a users' postcode is included in a fixed list of eligible postcodes. There are a large amount of postcodes to compare against (~400,000) and this check is done once per user.
Previously, the postcode list was included in a .cs file as part of the project, however we ran into issues where this file took a non-significant amount of time to compile, so we've had to rethink our approach.
The two main approaches we're considering are the following:
- Extracting the postcodes into a JSON file and loading this at runtime. We'd load the data into application memory once at startup (using a Singleton service) and check if a postcode is in this list
- Creating a new database table to hold our postcodes. This would be populated with the new data using a migration
Wonder what the thoughts are on the trade-offs between these two approaches? The database would certainly be more scalable though wonder for the amount of data being queried here whether it would justify the additional required setup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eligiblePostcodes = JsonConvert.DeserializeObject<List<string>>(jsonContents); | |
Previously, the postcode list was included in a .cs file as part of the project, however since upgrading to .NET 8 this makes the project take a significant amount of time (~50min) to compile, so we've had to rethink our approach. |
Otherwise I might be tempted to not suggest approaches and instead just ask what people would suggest? Can come in later with what we thought of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have swapped to using a resource file now based on the feedback
263f455
to
54fbd80
Compare
@@ -6,309 +6,297 @@ | |||
using GovUkDesignSystem.ModelBinders; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatted this file since there's an edit as part of this pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to not have all these changes mixed up in the same commit as the functional change. Could you go back and redo it, committing the formatting separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should now be separated
HerPublicWebsite/Startup.cs
Outdated
services.AddScoped<ICsvFileCreator, CsvFileCreator>(); | ||
services.AddScoped<IDataAccessProvider, DataAccessProvider>(); | ||
services.AddScoped<IEligiblePostcodeService, EligiblePostcodeService>(); | ||
services.AddSingleton<IEligiblePostcodeListService, EligiblePostcodeListService>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
functional change is this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move to the below section with the other singleton please? Easier to miss it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have moved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is effectively a cache, so we should name it as such: EligiblePostcodeListCache
. Then it's more naturally obvious why it's registered as a singleton.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is now called this
HerPublicWebsite/Startup.cs
Outdated
services.AddScoped<ICsvFileCreator, CsvFileCreator>(); | ||
services.AddScoped<IDataAccessProvider, DataAccessProvider>(); | ||
services.AddScoped<IEligiblePostcodeService, EligiblePostcodeService>(); | ||
services.AddSingleton<IEligiblePostcodeListService, EligiblePostcodeListService>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move to the below section with the other singleton please? Easier to miss it here.
public interface IEligiblePostcodeListService | ||
{ | ||
public List<string> GetEligiblePostcodes(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we actually benefit from having this interface do we? Or expect to meaningfully use it at any point. Would just drop it if it's boilerplate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have removed
@@ -6,309 +6,297 @@ | |||
using GovUkDesignSystem.ModelBinders; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to not have all these changes mixed up in the same commit as the functional change. Could you go back and redo it, committing the formatting separately?
9be8dfe
to
7820cfb
Compare
Link to Jira ticket
Description
follow on from PC-1049 #349, this ticket updates C# and .NET versions for the app.
following our initial findings, this ticket also moves the eligible postcodes to a JSON file where they are loaded at runtime
Checklist
Actions on Merge